-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH]message list: Preview of message when viewing message list #1241
base: master
Are you sure you want to change the base?
[ENH]message list: Preview of message when viewing message list #1241
Conversation
64a506d
to
3c81b19
Compare
98e3955
to
265c38c
Compare
@christer77 take this comment: #1159 (comment) into consideration too |
well noted |
265c38c
to
97a9b2a
Compare
97a9b2a
to
9171593
Compare
$struct = $imap->search_bodystructure($msg_struct, array('imap_part_number' => $part)); | ||
$msg_struct_current = array_shift($struct); | ||
$msg_text = $imap->get_message_content($uid, $part, $max, $msg_struct_current); | ||
$msg['preview_msg'] = $msg_text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you do it has several problems:
- Too many internal calls to imap method.
- Too many calls to imap server for each message - it will be rather slow for bigger lists of messages. Might slow down the normal list pages by 2-4 times.
- I think we should seriously do this conditionally based on a setting that user turns on and off for specific pages (imap folder pages, combined pages).
- The code to get the preview should be moved out of here as this only works for single imap folder pages. We have many other places listing messages (search results, combined pages - all, sent, unread, etc.)
- It is best to have a setting, pass it to the imap get_mailbox_page or get_message_list methods and have it query the text body as well if required. This will ensure we get all the content in a single call to the imap server. Please test this with 50 messages at once and you will notice the difference. Check if the IMAP protocol doesn't have something built-in - like getting part of the text body of the message or getting only the text body, not the whole structure. Parsing the bodystructure is an expensive operation. If we really need to do this for each message separately in list-view, we should consider aggressive caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
@@ -291,7 +296,7 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false, | |||
$res[$id] = message_list_row(array( | |||
array('checkbox_callback', $id), | |||
array('icon_callback', $flags), | |||
array('subject_callback', $subject, $url, $flags, $icon), | |||
array('subject_callback', array("subject" => $subject, "preview_msg" => $preview_msg), $url, $flags, $icon), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subject_callback is under your control - please add a separate parameter there for preview_msg instead of using subject and preview in an array - it is harder to understand what $vals[0] mean in the callback. Better use separate named parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted
Issue: Preview of message when viewing message list